Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attempt to fix MSFT_xIisModule #354

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

scottmckenzie
Copy link

@scottmckenzie scottmckenzie commented May 14, 2018

  • Changes to MSFT_xIisModule
    • Fix verb parsing.
    • Get fastCgi setting from webServer config.
    • Set handler setting on website.
    • Fix various typos.

Should fix #305 and #323


This change is Reviewable

@msftclas
Copy link

msftclas commented May 14, 2018

CLA assistant check
All CLA requirements met.

@codecov-io
Copy link

codecov-io commented May 14, 2018

Codecov Report

Merging #354 into dev will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@         Coverage Diff         @@
##            dev   #354   +/-   ##
===================================
  Coverage    89%    89%           
===================================
  Files        14     14           
  Lines      2210   2210           
===================================
  Hits       1983   1983           
  Misses      227    227

@johlju johlju added the needs review The pull request needs a code review. label May 14, 2018
@johlju
Copy link
Member

johlju commented May 14, 2018

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


a discussion (no related file):
Could you possible please add unit test(s) for these changes?


a discussion (no related file):
Could you please add an descriptive entry, for each fix, to the change log under the the Unreleased section in the file README.md? If an entry resolves an issue please reference the issue.


DSCResources/MSFT_xIisModule/MSFT_xIisModule.psm1, line 79 at r1 (raw file):

iis:\

Why not use Get-IisSitePath here as well? Maybe without the -SiteName parameter if it can only be found on "root level"?


DSCResources/MSFT_xIisModule/MSFT_xIisModule.psm1, line 163 at r1 (raw file):

$resourceTests.EndPointSetup

This should be $resourceStatus.EndPointSetup?


DSCResources/MSFT_xIisModule/MSFT_xIisModule.psm1, line 398 at r1 (raw file):

                    ModulePresent = $modulePresent
                    ModuleConfigured = $moduleConfigured
                    EndPointSetup = $resourceStatus.EndPointSetup

This should not be needed? See previous comment.


DSCResources/MSFT_xIisModule/MSFT_xIisModule.psm1, line 407 at r1 (raw file):

                    ModulePresent = $modulePresent
                    ModuleConfigured = $moduleConfigured
                    EndPointSetup = $resourceStatus.EndPointSetup

This should not be needed? See previous comment.


Comments from Reviewable

@johlju johlju added waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. and removed needs review The pull request needs a code review. labels May 14, 2018
- Add handler to specified site (dsccommunity#305).
- Parse verbs correctly (dsccommunity#323).
@johlju
Copy link
Member

johlju commented May 15, 2018

@scottmckenzie Could you please go into Reviewable and write 'Done' on the review comments that are resolved (or comment on them if they need to be discussed). After you replied to all review comments (they are saved as drafts), then press the big Publish button at the top of the page. That will send all review comments as one big comment back to this PR.

You find Reviewable in the big purple button in the PR description.

@scottmckenzie
Copy link
Author

Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.


DSCResources/MSFT_xIisModule/MSFT_xIisModule.psm1, line 79 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
iis:\

Why not use Get-IisSitePath here as well? Maybe without the -SiteName parameter if it can only be found on "root level"?

Done.


DSCResources/MSFT_xIisModule/MSFT_xIisModule.psm1, line 163 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
$resourceTests.EndPointSetup

This should be $resourceStatus.EndPointSetup?

Done.


DSCResources/MSFT_xIisModule/MSFT_xIisModule.psm1, line 398 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This should not be needed? See previous comment.

Done.


DSCResources/MSFT_xIisModule/MSFT_xIisModule.psm1, line 407 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This should not be needed? See previous comment.

Done.


Comments from Reviewable

@johlju
Copy link
Member

johlju commented May 16, 2018

The changes looks good, although it would be great if you could add a unit test for testing these changes (the last review comment). Unfortunately there are no existing unit test for this resource.
There is a template to use here; https://github.com/PowerShell/DscResources/blob/master/Tests.Template/unit_template.ps1

Let me know if you need any help.


Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from Reviewable

@scottmckenzie
Copy link
Author

Attempting to write some unit tests...

@johlju
Copy link
Member

johlju commented May 18, 2018

@scottmckenzie Awesome! If you get stuck let me know and I might help with pointers. 🙂

@stale
Copy link

stale bot commented Jun 18, 2018

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

@stale stale bot added the abandoned The pull request has been abandoned. label Jun 18, 2018
@johlju johlju changed the base branch from dev to master December 30, 2019 12:18
@johlju johlju changed the base branch from master to main January 7, 2021 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abandoned The pull request has been abandoned. waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

xlisModule Uses wrong configuration key/value pairs when adding handler config
4 participants